Skip to content

fix(config): never purge on a repeated delete; add restore + trash-list (0.89.0) - #643

Merged
padak merged 3 commits into
mainfrom
feat/config-delete-restore
Aug 22, 2026
Merged

fix(config): never purge on a repeated delete; add restore + trash-list (0.89.0)#643
padak merged 3 commits into
mainfrom
feat/config-delete-restore

Conversation

@padak

@padak padak commented Aug 22, 2026

Copy link
Copy Markdown
Member

The trap

The Storage API overloads DELETE .../configs/{id}: on a live configuration it soft-deletes into the trash, but on a configuration already in the trash the same call purges it permanently — versions, rows and metadata included. Verified against keboola/connection source: the official PHP client's purgeConfiguration docblock states it outright ("Unlike a repeated deleteConfiguration() call, this fails with 400 when the configuration is not in the trash, so a stale caller cannot destroy a live configuration") — the dedicated purge endpoint exists precisely because a repeated DELETE destroys.

A timed-out delete followed by a retry is exactly that second call. Retrying on timeout is what every agent and every CI script does. kbagent's goal is to be the CLI agents can be trusted with, so the CLI is where this gets closed.

What changed

config delete locates the configuration before deleting:

Found state Action Result
live DELETE status: "deleted"
already in trash no API write status: "already_in_trash", exit 0 — the retry stays idempotent
neither NOT_FOUND

The lookup needs two probes because a plain GET .../configs/{id} answers 404 for a trashed config and a missing one alike — only the isDeleted=true listing separates them. --dry-run reports the located state without writing.

Two new commands complete the loop:

  • config restorePOST .../configs/{id}/restore, the undo; versions, rows and metadata come back (write)
  • config trash-list — what restore can bring back, project-wide or --component-id-scoped (read)

All three mirrored on kbagent serve: DELETE gains dry_run; POST .../restore and GET /configs/trash/{project} are new.

The documentation finding

CLAUDE.md's All CLI Commands section had never listed config delete — the command existed for a long time, the inventory just omitted it. That is convention #17's silent drift in its purest form, and it misled an AI agent (me) earlier today into concluding the command didn't exist and advising deletion via the UI. Fixed here along with the double-delete gotcha in gotchas.md, commands-reference.md entries and AGENT_CONTEXT.

Structure

  • Trash lookup + shaping in services/_config_trash.py (config_service.py is over its size budget; the service methods stay thin — docstrings are free per loc-check)
  • Commands in commands/_config_trash_cmd.py, mounted via register() like _config_clone_cmd.py

Review round 2 — the hole Devin found in the above

The locate-first guard runs once per call, so it closes a retry across separate command invocations — but not the retry the HTTP client performs inside that one call. DELETE is in RETRY_SAFE_METHODS, so a read timeout or a 5xx made _do_request repeat it automatically, and on this endpoint the automatic repeat is the purge. That is the most likely form of the very scenario this PR set out to close, and the first commit left it open.

Reproduced before fixing: one delete_config call put two DELETEs on the wire.

  • _do_request gains a per-call retry_safe override (None keeps the method rule); client.delete_config sets False. A lost response now surfaces as TIMEOUT for the caller to decide about, and the re-run is safe because the service guard catches the trashed state — which a transport retry never can.
  • Every other DELETE keeps its retry. Idempotency is a property of the endpoint, not of the method.
  • locate_config no longer infers "live" from the absence of a 404: a body carrying isDeleted: true is read as trashed regardless of status code. (404-on-trashed is confirmed live on AWS and GCP stacks, but that flag decides whether a purge-capable DELETE goes out, so it is read rather than inferred.)

Tests

  • 15 unit tests — the client.delete_config.assert_not_called() assertions are the point of the file: they prove the purge call cannot happen for trashed/missing/dry-run/500-on-preflight paths
  • 3 router tests
  • E2E delete step now runs the full round trip live: delete → repeated delete answers already_in_trashtrash-list finds it → restoreconfig detail confirms live → final delete. This proves the guard against the real endpoint that overloads DELETE, not a mock.

Version

0.89.0 (pyproject + version-sync + changelog block). Pleasing detail: check_version_gates — added in #639failed this PR's own docs the moment (since v0.89.0) markers landed before the changelog block existed. First real catch.

make check green: 5875 passed, 12 skipped.

Open question (not in this PR)

config delete has no --yes confirmation while config row-delete does. Adding a TTY prompt now would break existing automation (the E2E suite itself calls it bare), so I left the semantics alone — worth a separate decision.


Open in Devin Review

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Open in Devin Review

Comment thread src/keboola_agent_cli/services/config_service.py Outdated
Comment thread src/keboola_agent_cli/services/_config_trash.py
padak added 3 commits August 22, 2026 22:57
…st (0.89.0)

The Storage API overloads DELETE .../configs/{id}: on a live configuration
it soft-deletes into the trash, but on a configuration ALREADY in the trash
the same call purges it permanently -- versions, rows and metadata included.
A timed-out delete followed by a retry is exactly that second call, and
retrying on timeout is what every agent and CI script does.

`config delete` now locates the configuration before deleting:

* live -> DELETE, status "deleted"
* already in the trash -> NO second DELETE, status "already_in_trash",
  exit 0 (the retry stays idempotent for scripts)
* absent from both -> NOT_FOUND (a plain GET 404s for trashed AND missing
  configs alike, so the trash listing is what separates the two)
* --dry-run reports the located state without writing

New commands complete the loop:

* `config restore` -- POST .../configs/{id}/restore, the undo (versions,
  rows and metadata come back); permission class write
* `config trash-list` -- what restore can bring back, project-wide or per
  component; permission class read

All three are mirrored on `kbagent serve` (DELETE gains dry_run;
POST .../restore and GET /configs/trash/{project} are new).

Docs: CLAUDE.md's All CLI Commands had NEVER listed `config delete` --
silent drift that made the command look nonexistent to AI agents reading
the file (it misled one today). Added, along with the double-delete gotcha
in gotchas.md and entries in commands-reference.md and AGENT_CONTEXT.

Endpoint semantics verified against keboola/connection source: the official
PHP client's purgeConfiguration docblock states the repeated-DELETE purge
behaviour and offers POST .../purge (400 when not trashed) as the safe
explicit path; restoreComponentConfiguration confirms the restore route.

Trash lookup and result shaping live in services/_config_trash.py because
config_service.py is over its size budget; the service methods stay thin.
Commands live in commands/_config_trash_cmd.py for the same reason
(config.py), mounted via register() like _config_clone_cmd.py.

15 unit tests (the assert_not_called() checks on client.delete_config are
the point: they prove the purge call cannot happen) + 3 router tests. The
E2E delete step now runs the full round trip live: delete -> repeated
delete answers already_in_trash -> trash-list finds it -> restore ->
detail confirms live -> final delete.

Version 0.89.0 (pyproject + version-sync), changelog block added -- which
is also what satisfies check_version_gates for the new (since v0.89.0)
markers; the gate caught this PR's own docs before the changelog existed.
The first commit pushed services/config_service.py from 1487 to 1566 code
lines, over the 1500 HARD ceiling -- make loc-check rightly blocked it.

- the delete/restore/trash-list bodies move fully into
  services/_config_trash.py (execute_delete / execute_restore /
  execute_trash_list); the ConfigService methods are resolve-and-delegate
  only
- _find_matches_in_json moves to json_utils.py as find_matches_in_json --
  it is a pure JSON-walking helper and json_utils is where those live
  (deep_merge, set_nested_value, compute_diff); the one call site and the
  test import follow

config_service.py lands at 1493 code lines: under the hard ceiling, still
carrying the pre-existing soft warning that a real split is due.
… a purge

Devin Review caught a hole in the first commit's central claim. The
locate-first guard runs ONCE per delete_config call, so it protects against
a retry across separate command invocations -- but not against the retry the
HTTP client performs inside that single call.

DELETE is in RETRY_SAFE_METHODS, so a read timeout or a 5xx made _do_request
repeat it automatically. On this endpoint the repeat IS the purge: the server
trashes the config, the response is lost, the retry lands on the now-trashed
config and destroys it before any caller sees a result. That is the MOST
likely form of the very scenario the PR set out to close, and it was still
open. Reproduced before fixing: one delete_config call put two DELETEs on the
wire.

- http_base._do_request gains a per-call `retry_safe` override; None keeps the
  method-based rule. _server_error_hint honours it too, so a 5xx on an
  opted-out call gets the "may already have taken effect" note.
- KeboolaClient._request passes it through; client.delete_config sets
  retry_safe=False. A lost response now surfaces as TIMEOUT for the caller to
  decide about, and re-running the command is safe because the service guard
  catches the trashed state -- which a transport-level retry never can.
- Every other DELETE keeps its retry. The opt-out is per endpoint because
  idempotency is a property of the endpoint, not of the method.

Second Devin point, also addressed: locate_config inferred "live" from the
absence of a 404. GET on a trashed config answers 404 on connection.keboola.com
and on the GCP stack (verified live), but a stack returning the tombstone body
would have made a 200 mean "trashed" -- and the DELETE that followed would
purge. It now reads `isDeleted` from the body rather than trusting the status
code, so the behaviour no longer depends on a cross-stack convention.

5 tests added (20 in the file): both purge paths register exactly ONE mocked
outcome, so a passing test proves a single DELETE left the client; plus the
default-still-retries case and the override in isolation.
@padak
padak force-pushed the feat/config-delete-restore branch from caaee67 to 01a364e Compare August 22, 2026 21:00
@padak
padak merged commit 7c7f05a into main Aug 22, 2026
4 checks passed
@padak
padak deleted the feat/config-delete-restore branch August 22, 2026 21:12
padak added a commit that referenced this pull request Aug 22, 2026
0.89.0 (config delete trash guard, #643) is merged on main but not yet
published, and changelog-check allows exactly one in-flight version -- so the
#646 entry joins the 0.89.0 release being prepared instead of stacking a
second unreleased bump on top. pyproject back to 0.89.0 (version-sync'd),
docs tags now read (since v0.89.0).
padak added a commit that referenced this pull request Aug 22, 2026
pyproject/plugin.json/marketplace.json were already renumbered to 0.89.0 by
v0.88.0.

Changelog: adds 0.89.0 entries for #645 (describe-batch --from-file shape
validation, issue #640), #642 (table-detail human column descriptions), #620
(sync-action forwards root authorization/runtime), #517 (stable metavar
contract, issue #513), #586 (documented prompt budget gated against the
enforced one, issue #585) and #641 (docs-only), and decorates the existing

Silent-drift surfaces:

* gotchas.md -- resolves both "(Release step: ... tag this sentence)"
  placeholders. Both were left by commits AFTER the v0.88.0 tag (#642 and
  #645), so both are tagged (since v0.89.0), not 0.88.0. Adds the #620 gotcha:
  below 0.89.0 a sync action on an OAuth / Service-Account component died with
  an opaque empty-body 400 because the broker reference was never forwarded.
* #620 shipped with no doc surfaces at all -- CLAUDE.md, AGENT_CONTEXT and
  commands-reference.md now carry the forwarding rule (root only, never
  row-overridden, only when non-empty) with its version gate.
* #645 never reached CLAUDE.md -- the describe-batch shape check and its
  behaviour change are recorded there now; commands-reference gains the
  version tag.
* #642's human Description column is version-tagged in CLAUDE.md,
  commands-reference.md, AGENT_CONTEXT and storage-describe-workflow.md.
* #643 was otherwise complete; adds the two surfaces it did not touch --
  safe-write-workflow.md (delete is reversible; never blind-retry on <= 0.88.x)
  and a keboola-expert.md matrix row for delete/restore/trash-list.
  keboola-expert.md is 49 774 B, well inside the 70 000 B budget.

make check green: 5934 passed, 12 skipped. version-gate-check resolves all 438
markers across 72 versions.
padak added a commit that referenced this pull request Aug 23, 2026
pyproject/plugin.json/marketplace.json were already renumbered to 0.89.0 by
v0.88.0.

Changelog: adds 0.89.0 entries for #645 (describe-batch --from-file shape
validation, issue #640), #642 (table-detail human column descriptions), #620
(sync-action forwards root authorization/runtime), #517 (stable metavar
contract, issue #513), #586 (documented prompt budget gated against the
enforced one, issue #585) and #641 (docs-only), and decorates the existing

Silent-drift surfaces:

* gotchas.md -- resolves both "(Release step: ... tag this sentence)"
  placeholders. Both were left by commits AFTER the v0.88.0 tag (#642 and
  #645), so both are tagged (since v0.89.0), not 0.88.0. Adds the #620 gotcha:
  below 0.89.0 a sync action on an OAuth / Service-Account component died with
  an opaque empty-body 400 because the broker reference was never forwarded.
* #620 shipped with no doc surfaces at all -- CLAUDE.md, AGENT_CONTEXT and
  commands-reference.md now carry the forwarding rule (root only, never
  row-overridden, only when non-empty) with its version gate.
* #645 never reached CLAUDE.md -- the describe-batch shape check and its
  behaviour change are recorded there now; commands-reference gains the
  version tag.
* #642's human Description column is version-tagged in CLAUDE.md,
  commands-reference.md, AGENT_CONTEXT and storage-describe-workflow.md.
* #643 was otherwise complete; adds the two surfaces it did not touch --
  safe-write-workflow.md (delete is reversible; never blind-retry on <= 0.88.x)
  and a keboola-expert.md matrix row for delete/restore/trash-list.
  keboola-expert.md is 49 774 B, well inside the 70 000 B budget.

make check green: 5934 passed, 12 skipped. version-gate-check resolves all 438
markers across 72 versions.
padak added a commit that referenced this pull request Aug 23, 2026
…es (#651)

Release prep for 0.89.0: adds the changelog entries for everything merged since v0.88.0 (#620, #642, #643, #644, #645, #646, #647, #648, #649, #650, #517, #586, #641), resolves every vNEXT placeholder left by feature PRs to v0.89.0 per the new #648 release process, closes the 10 gaps a full doc-surface audit found across the kbagent plugin (SKILL.md triggers, commands-reference, gotchas, workflow files, keboola-expert.md, AGENT_CONTEXT, CLAUDE.md), and records the live e2e verification evidence. Version files were already at 0.89.0 (bumped by #643); make version-sync is a no-op.
padak added a commit that referenced this pull request Aug 23, 2026
…ens, palette (#658)

NERD web UI catch-up to the 0.89.0 feature set, frontend + docs only. Jobs gain re-run (branch-preserving, config-aware) and terminate (gated to terminable statuses, ConfirmModal); config detail moves to a Drawer with Run job; configs get delete + a Trash tab with restore (#643); new Tokens page (list, opt-in derive-last-used with dormant-first sort, create/rotate/delete, secret shown once); Storage table drawer renders the BigQuery definition (#621) and column descriptions are click-to-edit (#624); Dashboard gains a PAYG credits tile; Flows gain a read-only Notifications tab (project-wide catch-alls grouped separately); ctrl/cmd+K command palette over pages, projects and actions. Fixes two long-standing UI bugs along the way: the jobs Config column read an invented configId key (API sends config — CLI twin tracked in #659) and ConfirmModal now portals to body. Cleanups: window.confirm removed, dead useManageTokenPrompt deleted. Frontend CI gate gap tracked in #660.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant